Add: report NEXT_LEVEL reservation stalls - #1613

tests/ut/cpp/hierarchical/test_scheduler.cpp (2)

1179-1186: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Assert the worker pointers before dereferencing them.

manager.get_worker_by_id returns nullptr for an unregistered id. Scheduler::dispatch_next_level_group in src/common/hierarchical/scheduler.cpp checks for that case explicitly. Worker ids 0 and 1 are registered in SetUp(), so the current test is safe. Add ASSERT_NE(..., nullptr) so a future registration change fails as an assertion instead of a segmentation fault inside a lock scope.

🛡️ Proposed change

         WorkerThread *manager_worker_a = manager.get_worker_by_id(WorkerType::NEXT_LEVEL, 0);
         WorkerThread *manager_worker_b = manager.get_worker_by_id(WorkerType::NEXT_LEVEL, 1);
+        ASSERT_NE(manager_worker_a, nullptr);
+        ASSERT_NE(manager_worker_b, nullptr);
         while ((!manager_worker_a->idle() || !manager_worker_b->idle()) &&

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/ut/cpp/hierarchical/test_scheduler.cpp` around lines 1179 - 1186, Add
ASSERT_NE checks for manager_worker_a and manager_worker_b immediately after
their get_worker_by_id calls and before the idle() polling loop, so null worker
registrations fail the test safely before dereferencing either pointer.

---

786-796: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Gate the diagnostic field checks with an assertion.

If the report does not arrive within 200 ms, line 790 fails and the test continues. Lines 791-796 then compare default-initialized capture fields and emit five more failures. Use ASSERT_EQ for the report-count check so the test stops at the root cause.

The same pattern applies to the exact dispatched_count() checks at lines 797, 810-811, and 820-821. Those are less severe because a stale count fails only one expectation.

♻️ Proposed change

-    EXPECT_EQ(stall_capture.report_count.load(std::memory_order_acquire), 1);
+    ASSERT_EQ(stall_capture.report_count.load(std::memory_order_acquire), 1);
     EXPECT_EQ(stall_capture.group_slot, group.task_slot);
-    EXPECT_EQ(stall_capture.busy_target_count, 1u);
+    ASSERT_EQ(stall_capture.busy_target_count, 1u);
     EXPECT_EQ(stall_capture.busy_target_worker_ids[0], 1);
-    EXPECT_EQ(stall_capture.idle_queued_target_count, 1u);
+    ASSERT_EQ(stall_capture.idle_queued_target_count, 1u);
     EXPECT_EQ(stall_capture.idle_queued_target_worker_ids[0], 0);
     EXPECT_EQ(stall_capture.idle_queued_single_head_slots[0], single_a.task_slot);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/ut/cpp/hierarchical/test_scheduler.cpp` around lines 786 - 796, Use
ASSERT_EQ for the stall_capture.report_count check before validating the
diagnostic fields, so the test returns immediately when no report arrives. Apply
the same assertion-strengthening to the exact dispatched_count() checks at the
indicated points, preserving their existing expected values and surrounding test
logic.

src/common/hierarchical/scheduler.h (2)

108-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Document the single-writer-thread invariant on reservation_stall_episode_.

reservation_stall_deadline() reads reservation_stall_episode_ under completion_mu_ (in run()), and update_reservation_stall() writes it under loop_mu_ (in dispatch_ready()). This is safe only because both calls execute exclusively on the scheduler thread. Add a short comment near the member to record this invariant, so a future change that invokes either method from another thread does not introduce a data race.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/common/hierarchical/scheduler.h` around lines 108 - 146, Add a short
comment immediately above reservation_stall_episode_ documenting that it is
accessed only by the scheduler thread, including its reads in
reservation_stall_deadline() and writes in update_reservation_stall(). Do not
change the locking or surrounding dispatch logic.

---

59-69: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Document the pointer-lifetime contract on ReservationStallDiagnostic.

busy_target_worker_ids, idle_queued_target_worker_ids, and idle_queued_single_head_slots are raw pointers. In scheduler.cpp, update_reservation_stall builds this struct from dispatch_result.busy_target_worker_ids.data() and similar calls, where dispatch_result is a temporary owned by the caller of dispatch_ready(). The sink call is synchronous, so the pointers stay valid only for the duration of that call.

Add a comment on the struct that states this constraint. A future sink implementation that stores these pointers past the callback invocation reads freed memory.

📝 Proposed documentation addition

     struct ReservationStallDiagnostic {
+        // All pointer/count fields below reference memory owned by the caller
+        // of the sink and are valid only for the duration of the sink call.
+        // Do not store these pointers past the callback invocation.
         TaskSlot group_slot{INVALID_SLOT};
         const int32_t *busy_target_worker_ids{nullptr};

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/common/hierarchical/scheduler.h` around lines 59 - 69, Add a
documentation comment to the ReservationStallDiagnostic struct stating that its
raw pointer fields are valid only during the synchronous ReservationStallSink
callback and must not be retained afterward, since they reference caller-owned
temporary storage.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes